feat!: produce a dual ESM/CJS build#401
Conversation
The global fetch API is built into Node 20+, browsers, Deno, Cloudflare Workers, and Vercel Edge. On modern versions of node taht use undici internally, this should result in improved perfomance. BREAKING CHANGE: The `$response` property type changes from `AxiosResponse<T>` to `FgaResponse<T>`. The constructor now accepts an optional `HttpClient` instead of `AxiosInstance`. `baseOptions.httpAgent`/`httpsAgent` are no longer applicable as fetch handles connection pooling natively.
…T" to avoid undefined request method, streaming timeout kills long-running streams
…cts. With fetch, createStreamingRequestFunction returns the raw ReadableStream directly — $response is never set, so the fallback was the only path that ever executed.
…rror properties Overwriting .constructor and .name on an error doesn't actually change its type, so instanceof checks failed for non-401/403 token endpoint errors.
…d of mutating error properties" This reverts commit e2e8878.
Inline async pool to remove it as the only CJS-style required runtime dependency. Info: about tiny-async-pool: - URL: https://github.com/rxaviers/async-pool - License: MIT - Author: Rafael Xavier de Souza | https://github.com/rxaviers
Separates crypto-free utilities from generate-random-id.ts (which imports from "crypto"). This allows consumers to import from utils/utils-lite without pulling in the crypto dependency. We faced this issue recently when we tried to use assertNever from this lib in syntaxt-transformer
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #401 +/- ##
===========================================
- Coverage 86.41% 86.24% -0.17%
===========================================
Files 25 27 +2
Lines 1354 1374 +20
Branches 263 263
===========================================
+ Hits 1170 1185 +15
- Misses 111 116 +5
Partials 73 73 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Conflicts: * errors.ts — main brought in PR #329's "stop mutating token refresh errors into auth errors" fix. Adapted for the fetch-era branch: FgaApiAuthenticationError now accepts HttpErrorContext | FgaApiError. When given an existing FgaApiError (re-wrap path from credentials/), it COPIES fields and preserves the original stack — the canonical fix for the audit-confirmed `instanceof` mutation P0. The HttpErrorContext path is unchanged. Also added the optional `context?: { clientId, audience, grantType }` second parameter from main so callers can override extracted values. * package.json — took main's bumped @opentelemetry/api ^1.9.1; left axios out (the whole point of this branch). * package-lock.json — regenerated via `npm install` after resolving package.json (don't hand-merge lockfiles). Removed two helper functions (parseRequestData, getAuthenticationErrorMessage) that main introduced for the AxiosError path; the fetch branch doesn't have AxiosError as a type, and the helpers had no other callers.
Pulls the fetch-based HTTP client + the FgaApiAuthenticationError re-wrap path (PR #329) into the WIP ESM/CJS dual-build branch. Conflicts: * package.json — kept ESM build setup (main / module / exports fields, build:cjs / build:esm / build:post scripts) AND took drop-axios's newer dep versions inherited from main: jest 30.3.0, ts-jest 29.4.9, eslint ^10.3.0, typescript ^6.0.3, nock ^14.0.14, @types/node ^25.6.0, @typescript-eslint/* ^8.59.2, @opentelemetry/api ^1.9.1. Removed tiny-async-pool + @types/tiny-async-pool (this branch already replaced tiny-async-pool with a native impl). * package-lock.json — regenerated via \`npm install\`. 0 axios entries, 0 tiny-async-pool entries, 0 vulnerabilities. * tests/apiExecutor.test.ts, tests/credentials.test.ts, tests/headers.test.ts — pure import-style conflicts. Kept this branch's \`.js\` extensions on every relative import (mandatory for ESM resolution) and drop-axios's default \`import nock from "nock"\` shape (nock 14's main entry is callable only via the default export; \`import * as nock\` works for namespace methods but breaks \`nock(url)\` calls). credentials.test.ts also kept the FgaApiAuthenticationError import that PR #329's tests need (it was lost on this branch when feat/esm-modules forked from the pre-#329 base). Files auto-merged: credentials/credentials.ts, errors.ts (which now includes PR #329's re-wrap branch in FgaApiAuthenticationError — typed \`HttpErrorContext | FgaApiError\` with optional context arg), tests/client.test.ts, tests/index.test.ts, tests/jest.config.js, tests/helpers/nocks.ts, tsconfig.json. Build (cjs + esm) clean. Tests: 302 passed across 15 suites.
Resolved conflicts by keeping the ESM-branch side in all cases: - .js extensions on relative imports (required for ESM resolution) - native utils/utils-lite/async-pool over tiny-async-pool dependency - ESM/CJS dual-build package.json fields - README Supported Runtimes section + ESM import examples package-lock.json regenerated via npm install (0 axios, 0 tiny-async-pool). Build (cjs+esm) clean. Tests: 302 passed across 15 suites.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK packaging to ship dual CommonJS + ES Module builds, adjusting internal import specifiers to be ESM-friendly and removing tiny-async-pool as a runtime dependency by inlining a compatible implementation.
Changes:
- Produce separate build outputs (
dist/cjs,dist/esm) and publish them viapackage.jsonexports(plus post-buildtypemarkers). - Update source + tests to use explicit
.jsspecifiers for ESM compatibility. - Inline lightweight utility helpers (including
asyncPool,chunkArray, header/property helpers) to reduce runtime dependencies.
Reviewed changes
Copilot reviewed 38 out of 44 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| validation.ts | Switch import specifiers to .js for ESM output compatibility. |
| utils/utils-lite/assert-never.ts | New lightweight utility export. |
| utils/utils-lite/async-pool.ts | Inline async concurrency pool to replace external dependency. |
| utils/utils-lite/chunk-array.ts | New chunking helper used by batching logic. |
| utils/utils-lite/set-header-if-not-set.ts | New header helper extracted into “lite” utils. |
| utils/utils-lite/set-not-enumerable-property.ts | New helper to define non-enumerable properties. |
| utils/utils-lite/index.ts | Barrel export for the new lite utilities. |
| utils/index.ts | Re-export from utils-lite + keep generate-random-id. |
| utils/generate-random-id.ts | Adjust random ID generation behavior (now always returns a string). |
| tsconfig.json | Emit CJS JS into dist/cjs while placing declarations under dist. |
| tsconfig.esm.json | New ESM build config emitting into dist/esm. |
| tests/validation.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/telemetry/metrics.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/telemetry/histograms.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/telemetry/counters.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/telemetry/configuration.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/telemetry/attributes.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/streaming.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/jest.config.js | Map .js specifiers back to TS sources for Jest resolution. |
| tests/index.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/helpers/nocks.ts | Update imports to .js to match ESM-style specifiers. |
| tests/helpers/index.ts | Update helper re-exports to .js. |
| tests/helpers/default-config.ts | Update imports to .js to match ESM-style specifiers. |
| tests/headers.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/fetch-http-client.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/errors.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/errors-authentication.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/credentials.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/client.test.ts | Update imports to .js to match ESM-style specifiers. |
| tests/apiExecutor.test.ts | Update imports to .js to match ESM-style specifiers. |
| telemetry/metrics.ts | Update internal imports to .js for ESM output compatibility. |
| telemetry/configuration.ts | Update internal imports to .js for ESM output compatibility. |
| README.md | Document CJS vs ESM usage and supported runtimes. |
| package.json | Add dual entrypoints + exports; split build into CJS/ESM + post step. |
| package-lock.json | Remove tiny-async-pool (and related typings); lockfile refresh. |
| index.ts | Update all exports to .js specifiers for ESM output compatibility. |
| errors.ts | Update import specifier + add HttpErrorContext interface. |
| credentials/index.ts | Update re-exports to .js. |
| credentials/credentials.ts | Update internal imports to .js for ESM output compatibility. |
| configuration.ts | Update internal imports to .js for ESM output compatibility. |
| common.ts | Update internal imports to .js for ESM output compatibility. |
| client.ts | Replace external tiny-async-pool require with in-repo asyncPool. |
| base.ts | Update internal imports to .js for ESM output compatibility. |
| api.ts | Update internal imports to .js for ESM output compatibility. |
Comments suppressed due to low confidence (1)
errors.ts:33
HttpErrorContextis declared twice, which will cause a TypeScript compile error (duplicate identifier). Remove the duplicate interface definition and keep a single source of truth.
/**
* Context extracted from a failed HTTP request/response,
* used to construct SDK error classes without coupling to any HTTP library.
*/
export interface HttpErrorContext {
status?: number;
statusText?: string;
headers?: Record<string, string>;
data?: any;
requestUrl?: string;
requestMethod?: string;
requestData?: any;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "compilerOptions": { | ||
| "module": "es2020", | ||
| "moduleResolution": "bundler", | ||
| "outDir": "dist/esm", | ||
| "declaration": false, | ||
| "declarationDir": null | ||
| } |
| /** | ||
| * Runs async functions in a limited concurrency pool. | ||
| * Yields results as they complete, in order of input. | ||
| * Rejects immediately if any promise rejects. | ||
| * | ||
| * @param concurrency - Maximum number of concurrent executions (>= 1) | ||
| * @param iterable - Input items to process | ||
| * @param iteratorFn - Async function to apply to each item | ||
| */ | ||
| export async function* asyncPool<IN, OUT>( | ||
| concurrency: number, | ||
| iterable: Iterable<IN>, | ||
| iteratorFn: (item: IN) => Promise<OUT>, | ||
| ): AsyncGenerator<OUT> { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const executing = new Set<Promise<any>>(); | ||
|
|
| import { TelemetryCounters } from "../telemetry/counters.js"; | ||
| import { TelemetryConfiguration } from "../telemetry/configuration.js"; | ||
| import { randomUUID } from "crypto"; | ||
| import SdkConstants from "../constants"; | ||
| import SdkConstants from "../constants/index.js"; |
| /** | ||
| * Generates a random ID | ||
| * | ||
| * Note: May not return a valid value on older browsers - we're fine with this for now | ||
| * Note: May not return a secure random value. | ||
| * We're fine with this, as this is just used to identify requests. |
- brace-expansion ^5.0.8 (fixes GHSA-mh99-v99m-4gvg high-severity DoS, advisory range <=5.0.7 also affects develop's lockfile) - @babel/core ^7.29.7 (fixes GHSA-4x5r-pxfx-6jf8 arbitrary file read) npm audit: 0 vulnerabilities. Build (cjs+esm) clean, 302 tests pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 44 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
tsconfig.esm.json:9
declarationDirexpects a string path; setting it tonullwill maketsc --project tsconfig.esm.jsonfail to parse/validate compiler options. Since declarations are disabled for the ESM build, this option should be removed.
utils/utils-lite/async-pool.ts:38- The docstring says results are yielded "in order of input", but this implementation yields in completion order via
Promise.race. Also,concurrencyis documented as ">= 1" but isn’t validated, which can lead to confusing behavior when callers pass 0/negative values.
| /** | ||
| * Context extracted from a failed HTTP request/response, | ||
| * used to construct SDK error classes without coupling to any HTTP library. | ||
| */ | ||
| export interface HttpErrorContext { | ||
| status?: number; | ||
| statusText?: string; | ||
| headers?: Record<string, string>; | ||
| data?: any; | ||
| requestUrl?: string; | ||
| requestMethod?: string; | ||
| requestData?: any; | ||
| } |
Description
This, coupled with dropping axios, should address #17 #72
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainInline async pool to remove it as the only CJS-style required runtime dependency.
Info: about tiny-async-pool: